Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement AES ECB, CBC and GCM modes #5

Merged
merged 8 commits into from
Jun 28, 2022
Merged

Implement AES ECB, CBC and GCM modes #5

merged 8 commits into from
Jun 28, 2022

Conversation

qmuntal
Copy link
Contributor

@qmuntal qmuntal commented Mar 16, 2022

Please review #3 first.

This PR implements AES ECB, CBC and GCM modes. Each mode is implemented in its own commit.

CRT mode is not supported by CNG so it has not bee implemented, see #4.

The AES GCM TLS mode has been implemented in the same way as OpenSSL: microsoft/go-crypto-openssl#21 as CNG does not have a specific TLS mode.

cng/aes.go Show resolved Hide resolved
cng/aes.go Show resolved Hide resolved
cng/aes.go Outdated Show resolved Hide resolved
cng/aes.go Outdated Show resolved Hide resolved
cng/aes.go Outdated Show resolved Hide resolved
cng/aes.go Show resolved Hide resolved
cng/aes.go Outdated Show resolved Hide resolved
Base automatically changed from dev/qmuntal/hmac to main March 24, 2022 18:24
@qmuntal qmuntal requested review from jaredpar and dagood March 29, 2022 08:07
Copy link
Member

@dagood dagood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's one small fix, but otherwise LGTM.

cng/aes.go Outdated Show resolved Hide resolved
if err != nil {
panic(err)
}
if int(ret) != len(src) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't int(ret) potentially truncate on 32 bit platforms?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ret is a uint32, so I don't think it will overflow. What will happen if len(src) don't fit in 32 bits is that this function will panic. I guess this behavior is right, at least I won't feel comfortable batching encrypts/decrypts in 32 bit chunks unless a crypto expert says we can do it.

Copy link
Member

@dagood dagood Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing to look at is signed values: https://go.dev/play/p/ZQXsT1GlEPQ

	var v uint64 = math.MaxUint64 // 64 to match platform-sized int in playground
	fmt.Printf("%x %x\n%x\n", v, int(v), math.MaxInt)
ffffffffffffffff -1
7fffffffffffffff

But I think it's ok here, because int is 32-bit, and len returns an int, then the size of the slice is limited to the positive 31 bits and this will never occur. If bcrypt.Decrypt is misbehaving and returns a large value that casts to a negative int, that will always panic because len(src) always returns a positivenon-negative int. (And we want a panic in this case.)

Co-authored-by: Davis Goodin <[email protected]>
@qmuntal qmuntal requested a review from jaredpar April 1, 2022 08:09
@qmuntal qmuntal merged commit d95b4e2 into main Jun 28, 2022
@qmuntal qmuntal deleted the dev/qmuntal/aes branch June 28, 2022 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants